CLI: polish three rough edges from the 1.3.7 prod shake-down#468
Open
CLI: polish three rough edges from the 1.3.7 prod shake-down#468
Conversation
End-to-end testing v1.3.7 against prod surfaced three quality-of-life
issues — none correctness bugs, but they all lead users (and agents)
into wrong conclusions. All three fixed centrally so future trash /
destructive endpoints inherit the right behavior for free.
### 1. Auth errors now exit 2 (was: exit 3)
Per CONTRIBUTING.md: 0=success, 1=error, 2=auth, 3=not-found.
Every \`rfapi.RoboflowError\` from a trash endpoint was being caught
with \`exit_code=3\`, so a script couldn't tell "your key is bad,
re-auth" apart from "this resource doesn't exist." Reproducer:
\$ roboflow -k bogus-key trash list --json ; echo \$?
{"error": ...}
3 ← should be 2
Fix:
- \`RoboflowError(message, status_code=None)\` now stores \`status_code\`
on the instance (defaulted to None for back-compat with the 30+
existing call sites that pass message-only).
- \`_raise_for_trash_response\` stamps \`response.status_code\` on the
raised exception so callers can branch on it.
- New \`output_api_error(args, exc, ...)\` helper in \`_output.py\` does
the central status→exit-code mapping (401→2, 404→3, else→1) plus
optional \`auth_hint\` / \`not_found_hint\` overrides.
- All four trash handlers (project/version/workflow/trash) replace
hardcoded \`output_error(..., exit_code=3)\` with \`output_api_error\`.
### 2. \`--json\` no longer bypasses the destructive-action prompt
Old logic was \`if not yes and not json: prompt\`. That conflated
*output formatting* with *destructive intent* — anyone piping
\`roboflow project delete X --json | jq\` got their project nuked
without any confirmation because \`--json\` was treated as "I'm in
non-interactive context, skip the prompt." Reproducer:
\$ roboflow project delete moondream-example --json < /dev/null
{"deleted": true, "trash": true, ...} ← deleted, no prompt!
Fix: new \`confirm_destructive(args, prompt)\` helper in \`_output.py\`
gates only on \`--yes\`/\`-y\` and TTY availability:
- \`--yes\` set → proceed.
- TTY without \`--yes\` → prompt (\`--json\` doesn't matter).
- No TTY without \`--yes\` → bail with exit 1 and a hint pointing at
\`--yes\` (would otherwise hang on \`typer.confirm\` reading a closed
stdin, or — in some runtimes — silently default through).
\`project delete\`, \`version delete\`, \`workflow delete\` all use it.
### 3. Idempotent re-delete returns success, not "missing scope"
When a project/version/workflow is already in Trash, the public API's
URL filter excludes it from the active set, so a follow-up \`DELETE\`
returns 404 ("Unsupported request. \`DELETE /ws/proj\` does not
exist..."). The CLI was surfacing that as exit 3 with the
\`Check your API key has 'project:update' scope\` hint, which is
factually wrong — the user has the scope, the item is just already
where they wanted it.
Fix: each delete handler now intercepts a 404 from \`rfapi.delete_X\`,
probes \`list_trash\`, and if the slug/url is found in the appropriate
\`sections\` array, emits a synthetic success payload that mirrors the
shape of a real successful delete plus an \`alreadyInTrash: true\`
marker. If the slug is genuinely not in trash either, the original
404 propagates as exit 3 (so typos still surface as errors).
Sample payload from a re-delete after first move:
{
"deleted": true,
"type": "project",
"workspace": "q3-board-meeting",
"project": "moondream-example",
"projectId": "Jjk3Tjq2yjxenyA9g5er",
"trash": true,
"alreadyInTrash": true
}
### Tests
\`tests/cli/test_trash_polish.py\` adds 14 tests covering all three:
- \`output_api_error\` mapping for 401 / 404 / 500 / no-status-code.
- \`_raise_for_trash_response\` actually attaches \`status_code\`.
- \`confirm_destructive\` honors \`--yes\`, refuses no-TTY without
\`--yes\`, ignores \`--json\` when deciding (regression guard), and
prompts via typer on a TTY (both accept and decline paths).
- Idempotent re-delete success path on project / version / workflow,
plus the negative case where a 404 with empty trash propagates as
exit 3 (so we don't mask real "not found" errors).
All 526 existing tests still pass; ruff + mypy clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68ac209753
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+24
to
+26
| def __init__(self, message, status_code=None): | ||
| super().__init__(message) | ||
| self.status_code = status_code |
There was a problem hiding this comment.
RoboflowError.__init__ now unconditionally assigns self.status_code = status_code, which defaults to None; this overwrites status codes set by subclasses that call super().__init__(message) without forwarding the second argument (notably ImageUploadError and AnnotationSaveError in rfapi.py). As a result, paths that raise ImageUploadError(..., status_code=...) or AnnotationSaveError(..., status_code=...) silently lose the HTTP code, regressing existing behavior and removing useful error context for retry/auth handling.
Useful? React with 👍 / 👎.
Contributor
digaobarbosa
left a comment
digaobarbosa
left a comment
There was a problem hiding this comment.
Quick suggestions to address the codex review detail.
Comment on lines
39
to
42
| self.message = message | ||
| self.status_code = status_code | ||
| self.retries = 0 | ||
| super().__init__(self.message) |
Contributor
There was a problem hiding this comment.
Suggested change
| self.message = message | |
| self.retries = 0 | |
| super().__init__(self.message, status_code=status_code) |
Comment on lines
31
to
34
| self.message = message | ||
| self.status_code = status_code | ||
| self.retries = 0 | ||
| super().__init__(self.message) |
Contributor
There was a problem hiding this comment.
Suggested change
| self.message = message | |
| self.retries = 0 | |
| super().__init__(self.message, status_code=status_code) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
End-to-end testing of the v1.3.7 CLI against prod (roboflow#11131 / roboflow-python#463 shipped this morning) surfaced three rough edges. None are correctness bugs — every command does the right server-side thing — but they all lead users (and especially agents) into wrong conclusions and were called out in the prod shake-down report. Worth fixing before the next release.
The three findings
1. Auth errors return exit 3 (should be 2)
Per
CONTRIBUTING.md:0=success, 1=error, 2=auth, 3=not-found. Everyrfapi.RoboflowErrorfrom the trash endpoints was caught withexit_code=3, so scripts can't differentiatere-authfromwrong slug:2.
--jsonsilently bypasses destructive-action confirmationOld logic was
if not yes and not json: prompt. That conflated output formatting with destructive intent — anyone piping… --json | jqgot their project nuked because--jsonwas treated as "I'm in CI, skip the prompt."3. Idempotent re-delete bubbles up a misleading "missing scope" 404
When a project/version/workflow is already in Trash, the public API's URL filter excludes it from the active set, so the next
DELETEreturns 404 ("Unsupported request.DELETE /ws/projdoes not exist..."). The CLI surfaced that as exit 3 withCheck your API key has 'project:update' scope— factually wrong (the user has the scope; the item is just where they wanted it).What
All three fixes live centrally so future trash / destructive endpoints inherit the right behavior for free.
Auth exit code (
#1)RoboflowError(message, status_code=None)now storesstatus_code. Defaulted toNonefor back-compat with the 30+ existing call sites that pass message-only._raise_for_trash_responsestampsresponse.status_codeon the raised exception.output_api_error(args, exc, hint=..., auth_hint=..., not_found_hint=...)helper incli/_output.pydoes the central status→exit-code mapping (401→2, 404→3, else→1). All four trash handlers (project / version / workflow / trash) replace hardcodedoutput_error(..., exit_code=3)with it.Destructive guard (
#2)confirm_destructive(args, prompt)incli/_output.py:--yes→ proceed.--yes→ prompt (regardless of--json).--yes→ bail with exit 1 + hint about--yes.project delete,version delete,workflow deleteall switched to it.Idempotent re-delete (
#3)rfapi.delete_X, probeslist_trash, and if the slug/url is found in the matchingsectionsarray, emits a synthetic success payload mirroring a real delete plusalreadyInTrash: true. If the slug is genuinely missing from trash too, the 404 propagates as exit 3 (typos still surface as errors).Sample payload from a re-delete:
{ "deleted": true, "type": "project", "workspace": "q3-board-meeting", "project": "moondream-example", "projectId": "Jjk3Tjq2yjxenyA9g5er", "trash": true, "alreadyInTrash": true }Tests
tests/cli/test_trash_polish.pyadds 14 tests covering all three:output_api_errormapping for 401 / 404 / 500 / no-status-code (defaults to 1, NOT 3 or 2 — important so legacyRoboflowError(text)callers don't accidentally claim "not found" / "auth")._raise_for_trash_responseactually attachesstatus_code.confirm_destructivehonors--yes, refuses no-TTY without--yes, ignores--jsonwhen deciding (regression guard for finding final changes to pip package #2), and prompts via typer on a TTY (both accept and decline paths).Backwards compatibility
RoboflowError(message)(no status_code) still works — every call site outside the trash endpoints does this and is unaffected.output_api_error,confirm_destructive) are additive in_output.py. Nothing else imports them yet.alreadyInTrashfield is additive for the previously-error-out 404 case, so any caller currently parsing a successful delete payload keeps working.Test plan
pytest/python -m unittest— all 526 tests pass.ruff check roboflow tests— clean.ruff format --check roboflow tests— clean.mypy roboflow— clean.alreadyInTrashpayload shape (mirrors successful delete fields verbatim —type,workspace,project/workflow,projectId/workflowId,trash).